Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

CosmosClientOptions: Adds validation for ApplicationName #3455

Merged
merged 18 commits into from
Sep 19, 2022
Merged

CosmosClientOptions: Adds validation for ApplicationName #3455

merged 18 commits into from
Sep 19, 2022

Conversation

NaluTripician
Copy link
Contributor

@NaluTripician NaluTripician commented Sep 12, 2022 •
edited
Loading

Pull Request Template

Description

User found that when ApplicationName contains an invalid value, the SDK is basically blocked (all requests fail). In order to prevent this, validation was added when setting application name that checks to see if it contains any illegal characters or values using the HttpHeaders.NameValueHeaderValue.Parse function. If found the SDK will throw an ArgumentException letting the user know that their ApplicationName contains illegal characters. Some of the aforementioned illegal characters can be found in the RFC1783 page 2 and 3. It should also be noted that although the "&" symbol there is said to be unsafe, through testing it appears to be a valid symbol to be used so the list of illegal characters for the SDK is slightly different than the ones mentioned in RFC1738, but the Parse function is able to tell which of these value are indeed unsafe.

Type of change

Please delete options that are not relevant.

  • [] Bug fix (non-breaking change which fixes an issue)
  • [] New feature (non-breaking change which adds functionality)

Closing issues

To automatically close an issue: closes #3040

github-actions[bot]
github-actions bot previously requested changes Sep 12, 2022
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please follow the required format: "[Internal] Category: (Adds|Fixes|Refactors|Removes) Description"

Internal should be used for PRs that have no customer impact. This flag is used to help generate the changelog to know which PRs should be included. Examples:
Diagnostics: Adds GetElapsedClientLatency to CosmosDiagnostics
PartitionKey: Fixes null reference when using default(PartitionKey)
[v4] Client Encryption: Refactors code to external project
[Internal] Query: Adds code generator for CosmosNumbers for easy additions in the future.

@NaluTripician NaluTripician changed the title IGNORE ME CosmosClientOptions: Adds validation that ApplicationName is valid for http requests Sep 13, 2022
@NaluTripician
Copy link
Contributor Author

I also want to note that like @ealsur mentioned in the issue the actual validation is not being done by the SDK therefore we cannot use the HttpHeaderParser.ParseValue method to check to see if the value will 100% work. When an ApplicaitonName is added, the final parsing is on not just the ApplicaitonName but additional text added by the HttpClient. For instance, if our application name is <Run>d__9 like the user had in their issue, the actual text the parser checks is 6.0.0-rc.2.21480.5|N|<Run>d__9. Although we are not checking the entirety of what the HttpHeaderParser does, I imagine that checking for these illegal characters will provide a fix for most if not all similar problems.

@ealsur
Copy link
Member

ealsur commented Sep 13, 2022

@NaluTripician The source of the problem is the HttpHeader failure, why did we revert that in place of the individual character validation? What is the benefit? Are there any characters that could still make the HttpHeader fail?

@ealsur ealsur changed the title CosmosClientOptions: Adds validation that ApplicationName is valid for http requests CosmosClientOptions: Adds validation for ApplicationName Sep 13, 2022
@NaluTripician
Copy link
Contributor Author

@NaluTripician The source of the problem is the HttpHeader failure, why did we revert that in place of the individual character validation? What is the benefit? Are there any characters that could still make the HttpHeader fail?

The previous function I was using was using in the HttpHeader class was not working however I have found the correct one and updated the code to use that.

FabianMeiswinkel
FabianMeiswinkel requested changes Sep 14, 2022
View reviewed changes
Copy link
Member

@FabianMeiswinkel FabianMeiswinkel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Substring(20) seems artificial on initial glance - needs some explanation or changes to the code to make it easier to understand.

FabianMeiswinkel
FabianMeiswinkel previously approved these changes Sep 14, 2022
View reviewed changes
Copy link
Member

@FabianMeiswinkel FabianMeiswinkel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

ealsur
ealsur previously approved these changes Sep 14, 2022
View reviewed changes
Copy link
Member

@ealsur ealsur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Just one small comment, please mark the PR ready for review

@ealsur ealsur mentioned this pull request Sep 14, 2022
Closed
@imanvt imanvt self-requested a review September 14, 2022 18:06
imanvt
imanvt previously approved these changes Sep 14, 2022
View reviewed changes
ealsur
ealsur previously approved these changes Sep 14, 2022
View reviewed changes
@NaluTripician NaluTripician marked this pull request as ready for review September 16, 2022 19:58
@ealsur ealsur dismissed github-actions[bot]’s stale review September 16, 2022 21:21

Bot not reacting correctly

@ealsur ealsur merged commit 5d70017 into master Sep 19, 2022
@ealsur ealsur deleted the users/nalutripician/ApplicationNameValidation branch September 19, 2022 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@imanvt imanvt imanvt left review comments

@FabianMeiswinkel FabianMeiswinkel FabianMeiswinkel left review comments

@ealsur ealsur ealsur approved these changes

@khdang khdang Awaiting requested review from khdang khdang is a code owner

@sboshra sboshra Awaiting requested review from sboshra sboshra is a code owner

@adityasa adityasa Awaiting requested review from adityasa adityasa is a code owner

@neildsh neildsh Awaiting requested review from neildsh neildsh is a code owner

@kirankumarkolli kirankumarkolli Awaiting requested review from kirankumarkolli kirankumarkolli is a code owner

@kirillg kirillg Awaiting requested review from kirillg kirillg is a code owner

@github-actions github-actions[bot] Awaiting requested review from github-actions[bot]

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Exception thrown when application name contains certain characters
4 participants
@NaluTripician @ealsur @FabianMeiswinkel @imanvt